fix(jira): add missing _raw_data_* columns to _tool_jira_sprint_reports - #9015
fix(jira): add missing _raw_data_* columns to _tool_jira_sprint_reports#9015DoDiODev wants to merge 1 commit into
Conversation
Integriert den Inhalt von PR apache#9015 in den lokalen Integrations-Branch. Die Sprint-Report-Migration 20260722_add_sprint_report_table.go legt _tool_jira_sprint_reports aus einem Struct an, das common.NoPKModel nicht einbettet, waehrend das Laufzeitmodell models.JiraSprintReport es einbettet. Die Spalten _raw_data_params / _raw_data_table / _raw_data_id / _raw_data_remark (plus created_at, updated_at) fehlten daher, sodass die Cleanup-Query des ApiExtractors WHERE _raw_data_table = ? AND _raw_data_params = ? den Subtask extractSprintReport mit "Error 1054 (42S22): Unknown column '_raw_data_table' in 'where clause'" abbrechen liess. Neue, additive Migration statt Aenderung der alten: Migrationsskripte sind append-only, und ein Edit wuerde Datenbanken nicht reparieren, die die Version bereits protokolliert haben. Zwei Schema-Drift-Guards, die die ECHTEN Migrationsskripte ausfuehren (statt das Laufzeitmodell zu AutoMigrate-n, was genau diese Drift verdecken wuerde): * plugins/jira/e2e/migration_schema_test.go - Jira-spezifisch. * plugins/schema_e2e/migration_schema_test.go - plugin-uebergreifend fuer alle Go-Plugins, inkl. TestAllGoPluginsListed. Der uebergreifende Guard deckte drei bestehende Drifts derselben Klasse auf, je mit eigener additiver Migration behoben: * _tool_taiga_scope_configs - type_mappings fehlte * _tool_teambition_scope_configs - id, created_at, updated_at fehlten * _tool_testmo_scope_configs - connection_id, name fehlten Verifiziert gegen MySQL 8.4.10 und PostgreSQL 17.2: neue Spalten werden nullable angelegt, doppelte NULLs sind im uniqueIndex zulaessig, und die AUTO_INCREMENT-PK laesst sich auf der PK-losen Teambition-Tabelle nachruesten (kein Error 1075). Alle vier Migrationen wurden zusaetzlich auf der bestehenden lokalen Datenbank angewendet. Ausserdem: plugins/schema_e2e in scripts/build-plugins.sh ausgeschlossen (kein Plugin, kein main-Package -> "make build-plugin" scheiterte mit "-buildmode=plugin requires exactly one main package"). AGENTS.md um die Schema-Drift-Guards und die zugehoerigen Fallstricke ergaenzt (fork-only, nicht Teil des Upstream-PRs). Signed-off-by: DoDiODev <DoDiDev@proton.me>
|
Hi. Could you please take a look at the failing test cases. Thanks. |
The Sprint Report migration 20260722_add_sprint_report_table.go creates
_tool_jira_sprint_reports from a struct that does not embed
common.NoPKModel, while the runtime model models.JiraSprintReport does.
The columns _raw_data_params / _raw_data_table / _raw_data_id /
_raw_data_remark (plus created_at, updated_at) were therefore never
created, so the ApiExtractor cleanup query
WHERE _raw_data_table = ? AND _raw_data_params = ?
made the extractSprintReport subtask fail with
"Error 1054 (42S22): Unknown column '_raw_data_table' in 'where clause'".
Add a new, additive migration that re-runs AutoMigrateTables on a struct
embedding archived.NoPKModel. The original migration is left untouched:
migration scripts are append-only, and editing it would not repair
databases that already recorded its version.
Add two schema-drift regression guards that run the REAL migration
scripts instead of AutoMigrate-ing the runtime model, which would hide
this class of drift:
* plugins/jira/e2e/migration_schema_test.go - Jira-specific guard.
* plugins/schema_e2e/migration_schema_test.go - cross-plugin guard for
every built-in Go plugin, including TestAllGoPluginsListed so the
guard stays complete when a new plugin is added.
Both guards run the migrations against a dedicated, empty database
created by the new helper e2ehelper.NewIsolatedMigrationDb: the shared
e2e database is polluted by the other e2e tests, which AutoMigrate
tables without recording anything in _devlake_migration_history, so
running the real scripts against it fails with errors such as
"Table 'cicd_pipeline_commits' already exists".
The cross-plugin guard immediately uncovered three pre-existing drifts
of the same class, each fixed with its own additive migration:
* _tool_taiga_scope_configs - missing type_mappings
* _tool_teambition_scope_configs - missing id, created_at, updated_at
* _tool_testmo_scope_configs - missing connection_id, name
The teambition table has no primary key at all, and its missing `id` is
an auto-increment primary key, which AutoMigrate cannot append to an
existing table (MySQL: "Incorrect table definition; there can be only
one auto column and it must be defined as a key"). That column is
therefore added with explicit DDL, which also keeps the ids of existing
rows and the sequence/counter in sync on both MySQL and PostgreSQL.
Finally, exclude plugins/schema_e2e from scripts/build-plugins.sh: it is
not a plugin and has no main package, which broke `make build-plugin`
with "-buildmode=plugin requires exactly one main package".
Signed-off-by: DoDiODev <DoDiDev@proton.me>
d2491ee to
db538d3
Compare
Follow-up fixes from the review of apache#9015: - add e2ehelper.NewIsolatedMigrationDb so the schema-drift guards run the real migration scripts against a dedicated, empty database instead of the shared E2E_DB_URL one - use it in the jira and cross-plugin schema-drift tests - fix the teambition scope-config migration accordingly - document the findings in AGENTS.md
|
Nice piece of work — particularly leaving One gap worth considering in the new guard, since it's the most interesting part of this PR.
On an empty Would it be worth inserting a row into each table before the tail migrations run, so the guard also asserts the migration applies to a non-empty table? Even a single row would have made the Not a blocker — the fix itself looks correct on both engines to me. |
Problem
Collecting Jira data fails in the
extractSprintReportsubtask with:Root cause
The Sprint Report feature (PR #8967 / #9010) added the table
_tool_jira_sprint_reports. Its migration(
20260722_add_sprint_report_table.go) creates the table from a struct thatdoes not embed
common.NoPKModel:The runtime model
models.JiraSprintReportdoes embedcommon.NoPKModel(→
common.RawDataOrigin), so it expects the columns_raw_data_params,_raw_data_table,_raw_data_id,_raw_data_remark(pluscreated_at,updated_at).During extraction,
api.NewApiExtractordeletes outdated rows viaWHERE _raw_data_table = ? AND _raw_data_params = ?(
helpers/pluginhelper/api/batch_save_divider.go). Because the migration nevercreated those columns, MySQL rejects the query with error 1054.
What changed
1. Fix for the reported bug (jira)
plugins/jira/models/migrationscripts/20260727_add_raw_data_columns_to_sprint_report.gore-runs
AutoMigrateTableson a struct that embeds the raw-data columns,adding them to existing
_tool_jira_sprint_reportstables without dataloss (GORM
AutoMigrateonly adds missing columns). Registered inmodels/migrationscripts/register.go.20260722_add_sprint_report_table.gois left unchanged:migration scripts are append-only, and editing it would not repair databases
that already ran it (its version is already recorded in
_devlake_migration_history).2. Regression tests (schema-drift guards)
plugins/jira/e2e/migration_schema_test.go— runs the real Jira migrationchain (framework + jira) and asserts every column each Jira model declares
exists in the migrated table. Directly reproduces and guards the bug above.
plugins/schema_e2e/migration_schema_test.go— cross-plugin generalization:applies framework + all plugin migrations and validates model-vs-table
column parity for every built-in Go plugin. Includes
TestAllGoPluginsListed,which fails if a new plugin directory with an
implpackage is added but notregistered, so the guard stays complete automatically.
Both deliberately run the real migration scripts instead of
AutoMigrate-ingthe runtime model — an
AutoMigrate-based check could never detect this class ofdrift. Both live in
e2epackages, so they run undermake e2e-test-go-plugins(require
E2E_DB_URL) and are excluded from the DB-less unit-test run.Implementation details worth noting:
dalgorm.Init(...)to register theencdecGORM serializer.runner.CreateBasicResdoes not do this (onlyCreateAppBasicResdoes),so without it the migrations abort with
invalid serializer type encdec.ENCRYPTION_SECRETif none is configured;some migrations (e.g. jira
20220716) refuse to run without one(
jira v0.11 invalid encKey), and CI does not provide a value._tool_jira_server_infos) are skipped — the check targets drift between anexisting table and its model.
3. Additional schema drifts found by the new cross-plugin guard
The guard immediately uncovered three pre-existing bugs of exactly the same
class. Each is fixed with a new, additive migration (registered in the
respective
register.go):_tool_taiga_scope_configstype_mappings_tool_teambition_scope_configsid,created_at,updated_at_tool_testmo_scope_configsconnection_id,nameFiles:
plugins/{taiga,teambition,testmo}/models/migrationscripts/20260727_add_missing_scope_config_columns.go.All new migration scripts use
core/models/migrationscripts/archived— importingcore/models/commonfrom a migration script is rejected bymake migration-script-lint.Why these repairs are safe on populated tables
The scope-config repairs re-add columns that carry constraints in
common.ScopeConfig(namehas auniqueIndex) and, for teambition, anAUTO_INCREMENT primary key. Both were verified against live engines rather than
assumed:
uniqueIndexonname— GORM adds new columns as nullable(
ALTER TABLE ... ADD COLUMN name VARCHAR(255), noNOT NULL DEFAULT ''), sopre-existing rows are backfilled with
NULL, and both MySQL and PostgreSQLpermit duplicate
NULLs in a unique index. Creating the index on a table withtwo pre-existing rows succeeded on MySQL 8.4.10 and PostgreSQL 17.2.
The counter-test confirms the distinction: with an explicit
NOT NULL DEFAULT ''column the same index fails(
Error 1062/pq: … (23505)) — which is exactly the case that does notoccur here.
idon the primary-key-less_tool_teambition_scope_configs—GORM issues a plain
ADD COLUMN, and MySQL backfills consecutive ids for theexisting rows (verified: two pre-existing rows received
id1 and 2). NoError 1075("there can be only one auto column and it must be defined as akey").
4. Build script
scripts/build-plugins.shbuilds every directory underplugins/with-buildmode=plugin.plugins/schema_e2e/is not a plugin (it contains only thecross-plugin test and has no
mainpackage), which mademake build-pluginfailwith "-buildmode=plugin requires exactly one main package". The directory is now
excluded, alongside the existing
core/helper/logsexclusions.Why a new migration (not editing the old one)
20260722is already onmain/upstream/mainand hasalready been applied to real databases; its version is recorded, so it will
never re-run. Only a new migration can repair those databases.
environments.
Testing
make migration-script-lint— OK.gofmt -lon all added/changed files — clean.go vet ./plugins/{jira,taiga,teambition,testmo,schema_e2e}/...— OK.go test ./plugins/schema_e2e/— OK, 42/42 plugins PASS(
TestAllGoPluginsListedincluded).go test -run TestMigrationSchema ./plugins/jira/e2e/— OK.tables contain the added columns.
migrations were applied to a real, long-running DevLake instance. They are
recorded in
_devlake_migration_historyand the target tables carry theexpected columns afterwards — e.g.
_tool_teambition_scope_configsgainedid,created_at,updated_at, and_tool_jira_sprint_reports(661 rows)gained the
_raw_data_*columns without data loss.verified separately against MySQL 8.4.10 and PostgreSQL 17.2 — see
§3 "Why these repairs are safe on populated tables".
_raw_data_tablefrom_tool_jira_sprint_reportsmakes the cross-plugin guard fail with
[jira] table "_tool_jira_sprint_reports" is missing column "_raw_data_table"— i.e. the guard genuinely detects the original regression.
Known limitations of the guard (intentional)
primary keys or indexes.
FlushTabler(drop +AutoMigrateof the runtime model), so a table touched byan earlier test can appear "repaired". On a fresh database (as in CI) this does
not apply.
GetTablesInfo()but no table at all" is not flagged.